applications/portfoliomanager/src/portfoliomanager/preprocess.py (1)

7-9: Make thresholds configurable via parameters (repeat of earlier feedback)

Hardcoded thresholds make experimentation cumbersome. Thread them as optional parameters with sensible defaults to preserve current behavior.

-def filter_equity_bars(data: pl.DataFrame) -> pl.DataFrame:
+def filter_equity_bars(
+    data: pl.DataFrame,
+    minimum_average_close_price: float = 10.0,
+    minimum_average_volume: float = 1_000_000.0,
+    *,
+    strict: bool = True,
+) -> pl.DataFrame:
     data = data.clone()
 
-    minimum_average_close_price = 10.0
-    minimum_average_volume = 1_000_000.0
+    price_col = pl.col("avg_close_price")
+    vol_col = pl.col("avg_volume")
+    price_pred = price_col > minimum_average_close_price if strict else price_col >= minimum_average_close_price
+    vol_pred = vol_col > minimum_average_volume if strict else vol_col >= minimum_average_volume
 
     return (
         data.group_by("ticker")
         .agg(
             avg_close_price=pl.col("close_price").mean(),
             avg_volume=pl.col("volume").mean(),
         )
-        .filter(
-            (pl.col("avg_close_price") > minimum_average_close_price)
-            & (pl.col("avg_volume") > minimum_average_volume)
-        )
+        .filter(price_pred & vol_pred)
     )

applications/portfoliomanager/pyproject.toml (1)

8-11: Packaging enabled: good step; ensure direct runtime deps are declared

Enabling [tool.uv].package = true means portfoliomanager can be built/installed independently. Since portfoliomanager.preprocess imports polars, declare polars as a direct dependency here instead of relying on transitive installs via internal to prevent import errors for downstream consumers.

Apply:

 [project]
 name = "portfoliomanager"
 version = "0.1.0"
 description = "Portfolio prediction and construction service"
-requires-python = "==3.12.10"
-dependencies = ["internal"]
+requires-python = ">=3.12,<3.13"
+dependencies = [
+  "internal",
+  "polars",  # direct runtime dependency used by portfoliomanager.preprocess
+]

applications/portfoliomanager/tests/test_preprocess.py (1)

1-4: Minor nit: prefer DataFrame.height for row counts

len(df) works, but df.height reads clearer and avoids constructing an iterator in some backends. Optional.

Example:

-assert len(result) == 1
+assert result.height == 1

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

applications/portfoliomanager/src/portfoliomanager/preprocess.py (1)

4-20: Solid, side-effect-free filtering; aligns with tests

Cloning the input for immutability and using per-ticker aggregates with strict “greater-than” thresholds is clear and easy to reason about. The returned schema (ticker, avg_close_price, avg_volume) matches the tests.

libraries/python/src/internal/tft_dataset.py (5)

30-31: Good call to clone the input

Defensive copy avoids mutating upstream dataframes during feature engineering.

---

147-154: Explicit Int64 casts for calendar features improve dtype stability

Casting calendar columns to Int64 ensures deterministic dtypes across platforms and Polars versions.

---

165-166: Schema validation placed pre-scaling: good

Validating full, engineered-frame dtypes before scaling/mapping will surface data issues early and keeps the scaler’s inputs consistent.

---

140-143: Timezone change to UTC requires downstream validation

The recent switch in tft_dataset.py from using America/New_York–derived midnight to UTC for timestamp normalization may shift epoch seconds by up to 5 hours (depending on DST), which can impact any downstream logic or consumers built around NY-local dates. Please review the following areas where America/New_York–based timestamps are explicitly used and confirm that their behavior remains correct:

• libraries/python/src/internal/cloud_event.py (lines 16–20, 33–37): event “date” fields generated with ZoneInfo("America/New_York").
• libraries/python/src/internal/dates.py (lines 9–13, 21–25): default_factory and parser .replace(tzinfo=ZoneInfo("America/New_York")).
• libraries/python/tests/test_dates.py (lines 11–15): tests mocking datetime.now(tz=ZoneInfo("America/New_York")).
• applications/models/src/models/combine_tft_data_sources.py (lines 40–44): output filename timestamp uses NY timezone.
• applications/models/src/models/train_tft_model.py (lines 20–30, 44–53, 87–95, 100–108, 123–131, 141–142): logging runtimes and run names all use ZoneInfo("America/New_York").
• applications/models/src/models/get_alpaca_equity_bars.py (lines 61–63): end = datetime.now(tz=ZoneInfo("America/New_York")).
• applications/datamanager/src/datamanager/alpaca_client.py (lines 102–114): converts naive timestamps to UTC then compares dates in NY to determine bar grouping.
• applications/datamanager/tests/test_alpaca_client.py (lines 36–44, 72–76, 130–138): tests assume bar timestamps at NY 16:00 local.

Double-check each consumer’s use of epoch seconds or date comparisons to ensure no unintended shifts occur now that tft_dataset uses UTC normalization.

---

156-163: time_idx bump to Int64 is safe — no Int32 consumers found

I’ve verified that:

• The only definitions and uses of time_idx are within tft_dataset.py, and it’s now consistently cast to pl.Int64.
• The Pandera schema defines "time_idx": pa.Column(int, required=True), which maps to Python’s int (i.e. NumPy/Pandas int64) rather than a 32-bit type.

No other code paths serialize or consume time_idx as a 32-bit integer. You can safely keep the Int64 change.

applications/portfoliomanager/tests/test_preprocess.py (3)

6-25: Tests hit the intended grouping/aggregation behavior

Covers strict “greater-than” semantics and validates both aggregate values and output schema. Nice.

---

73-114: Boundary and near-boundary coverage is thorough

Exact-threshold exclusion and “just above” inclusion guard against off-by-one mistakes.

---

197-214: Immutability test is valuable

Confirms the clone pattern is honored and protects callers from accidental mutation.

libraries/python/src/internal/tft_dataset.py (4)

140-143: Use dt.epoch('ms') for clarity and unit safety when deriving fallback timestamps

Casting to Int64 and dividing by 1,000 relies on the engine’s default time unit. dt.epoch('ms') makes the intent explicit and robust.

-                    .cast(pl.Int64)
-                    .floordiv(1000)
+                    .dt.epoch("ms")

---

165-165: Good: validate with schema before scaling

Validating prior to scaling prevents dirty data from contaminating the scaler. Consider strict=True (see next comment) if you want to forbid extra columns creeping in.

---

330-382: Tighten schema: set strict=True and align with upstream equity schema to avoid drift

• Add strict=True to reject unexpected columns.
• Optional: add ordered=False explicitly for clarity (defaults are fine).
• Ensure this schema stays in sync with any upstream equity_bar_schema to avoid silent divergence in constraints.

Proposed minimal change:

-dataset_schema = pa.DataFrameSchema(
-    {
+dataset_schema = pa.DataFrameSchema(
+    {
         # columns...
-    }
-)
+    },
+    strict=True,
+)

If upstream uses different dtypes (e.g., volume as float), decide on one canonical schema and convert at the ingestion boundary.

---

86-101: Scope of backfilling: avoid creating rows outside each ticker’s active date range

Using global min/max dates and a cross join will create rows for dates where a ticker didn’t exist/list—later filled by forward-fill/zeros—introducing long tails of synthetic data. Prefer per-ticker date ranges:

• Compute per-ticker min/max dates.
• Join each ticker to its own date range.

If you’d like, I can draft a per-ticker date_range construction using Polars’ group-wise operations.

Also applies to: 97-101, 123-123

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

libraries/python/src/internal/tft_dataset.py (4)

1-3: Imports look good; Polars + Pandera(polars) + date type are correctly wired

Nothing to change here; the date import is needed for the schema and pandera.polars is the right module.

---

30-31: Nice: cloning input avoids side effects

Cloning data up front prevents subtle caller mutations. Good call.

---

147-154: Confirm Zero-Based day_of_week Representation

I’ve audited all occurrences of dt.weekday() and the day_of_week column in the repo—both reside in tft_dataset.py:

• libraries/python/src/internal/tft_dataset.py
  • Lines 103–107: computes a temporary weekday via pl.col("date").dt.weekday().alias("temporary_weekday")
  • Lines 147–152: assigns day_of_week = pl.col("date").dt.weekday().alias("day_of_week")

No offsets (+1) or alternative conventions were found elsewhere. If your downstream embeddings or persisted models expect Monday = 1…Sunday = 7, please adjust these to dt.weekday() + 1. Otherwise, consider adding a brief doc comment (in the function docstring or module README) to clarify that day_of_week is zero-based, ensuring consistency across training and inference.

---

156-163: time_idx indexing is acceptable as-is

After reviewing the downstream usage of time_idx—sorting by it in the batching logic (around line 295) and declaring it in the schema (around line 380)—there are no existing consumers expecting a 0-based index. If you prefer 0-based numbering for array-style consistency, you can adjust the rank:

- .rank("dense")
+ .rank("dense") - 1

However, since no parts of the code assume 0-based indexing, the current 1-based implementation is fine.